-
Couldn't load subscription status.
- Fork 54
Clarify when details should go on events vs targets #585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify when details should go on events vs targets #585
Conversation
| What is more often implied by "asynchronous event" is to defer firing an event. | ||
|
|
||
| <h3 id="state-and-subclassing">Use plain {{Event}}s for state</h3> | ||
| <h3 id="state-and-subclassing">Put state on {{Event/target}} objects rather than {{Event}}s</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous heading seemed… incomplete.
|
Thanks for this I meant to raise an issue that examples should be added. I think an example that follows the principle is good but an example that doesn't follow it, and what it should have looked like would be useful too. |
index.bs
Outdated
|
|
||
| Defining an interface also exposes it on the global scope, allowing for the specification of static methods. | ||
| For example, the `canParse()` static method of the URL interface. | ||
| For example, the `canParse()` static method of the URL interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change, but it's something many editors do automatically. I can remove it, but I imagine others will run into the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are not the only one. We should maybe lint for trailing whitespace.
index.bs
Outdated
| Instead, all state should have been on the {{XMLHttpRequest}} object, | ||
| and the {{XMLHttpRequestEventTarget/progress}} event should have been a simple {{Event}}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree with this principle. This assumes that tracking this information is free. I don't think it necessarily is. And with the current design only those interested in the information pay for having it tracked.
(This doesn't mean that FetchObserver/FetchMonitor can't expose the information directly, because obtaining such an object could be considered the opt-in.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is way better than what we had. Thanks for doing this. I've made a few suggestions, but feel free to dismiss them.
index.bs
Outdated
| and use events to signal updates to that state. | ||
|
|
||
| It's usually not necessary to create new subclasses of {{Event}}. | ||
| <div class="note"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be a note?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it to separate the rule from the explanation of the rule, since the explanation is non-normative. But this spec doesn't have such a strict divide between normative and non-normative. But I don't mind either way, happy to go with whatever you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand this paragraph...
Or if it's completely accurate. There are a few events that are fired asynchronously, e.g. slotchange. The state on the event target might have been updated when the event listener is triggered.
Co-authored-by: Martin Thomson <[email protected]>
|
I've removed the XHR counter-example based on @annevk's feedback. I couldn't immediately think of another counter-example. Happy for this to land once others are. |
|
I think the XMLHttpRequest example showed a problem with this principle as a whole. That it doesn't properly consider the engineering trade-offs involved. Whether you expose state on the |
|
My assumption was that any rules in this doc can be broken if it can be demonstrated that the alternative is better for users/developers in a given case. @annevk would it help if I added a note recognising the case where the event listener can be used as an opt-in to internal processing? Although, as we see in the fetch case, there are other ways to handle the opt-in. |
|
Yeah, maybe something that hints at the fact that for potentially costly auxiliary state there is a real trade-off to be made here. While principles can be broken, generally they point out the one correct way to do something and folks might well take away from that they can never use state on events. |
|
@annevk PTAL |
index.bs
Outdated
| Although, other patterns should be considered, | ||
| such as retuning an {{EventTarget}} from a function, | ||
| where the function call is a similar signal of interest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that I'm following this sentence. I don't know what "retuning" means in this context.
Do you mean that people might want to consider adding a function to EventTarget to get the state instead? Or are you saying that you might add a function that flips the EventTarget into a mode that tracks the state, so that the cost is only paid when the state is needed?
It would really help if there was a concrete example of this problem. I'm having a little trouble coming up with one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling the function returns the EventTarget, at which point the implementation will have to track the state (if it's exposed on the target instead of the event).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so instead of something like:
self.pageDataUsage.addEventListener('change', () => {
console.log(self.pageDataUsage.bytesReceived);
});…where self.pageDataUsage.bytesReceived will need to be continually updated so it can be synchronously accessed by any code running in the doc/worker, you'd do something like:
self.pageDataUsage.addEventListener('change', (event) => {
console.log(event.bytesReceived);
});…where the data is only available to listeners of the change event, so if there are no listeners, the underlying operation to get bytesReceived doesn't need to happen. Or, alternatively, "return an {{EventTarget}} from a function":
const pageDataUsage = await self.monitorDataUsage();
pageDataUsage.addEventListener('change', () => {
console.log(pageDataUsage.bytesReceived);
});In this case, it's the call to monitorDataUsage() that signals interest, and GC of pageDataUsage will signal disinterest.
The second model gives you an object representing current state that can be passed around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retuning is a typo in the PR. I'll fix that once we figure out the right wording.
ddb59f6 to
9d24b6b
Compare
|
I've updated this with an example. Does it make more sense now? |
index.bs
Outdated
| Where `bytesReceived` exists on the `dataUsage` {{EventTarget}}, | ||
| rather than the {{Event}}. | ||
|
|
||
| If updating the state on this object is too expensive, this pattern may be considered: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| If updating the state on this object is too expensive, this pattern may be considered: | |
| If maintaining the state on this object is too expensive, this pattern may be considered: |
This approach still assumes that the dataUsage.bytesReceived is maintained, but only when you ask for it. A truly expensive API might use:
self.addEventListener('dataUsage', e => {
console.log(e.bytesReceived);
});No need to include that though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't that be equally expensive as the function example? I guess removeEventListener is a more immediate signal of disinterest vs waiting for GC on the object you get back from the function.
Co-authored-by: Martin Thomson <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
Co-authored-by: Martin Thomson <[email protected]>
|
I'm happy for this to land |
|
So am I. Thanks Jake. |
SHA: 79c2b78 Reason: push, by martinthomson Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I'm proposing this following discussions on
ProgressEventin webmachinelearning/translation-api#61.I wanted to provide reasoning and clarity for this rule, assuming I understand its intent correctly.
Preview | Diff